Skip to content

Make coverage reporting actually run, and send it to Codecov - #964

Merged
Carreau merged 6 commits into
mainfrom
claude/coverage-codecov-setup-892zlt
Aug 3, 2026
Merged

Make coverage reporting actually run, and send it to Codecov#964
Carreau merged 6 commits into
mainfrom
claude/coverage-codecov-setup-892zlt

Conversation

@Carreau

@Carreau Carreau commented Aug 3, 2026

Copy link
Copy Markdown
Member

Summary

The coverage job on main has never reported anything. Its log:

+ python -Iim coverage combine
No data to combine
+ python -Iim coverage report --fail-under=80
No data to report.

Two separate bugs in jupyterlab/maintainer-tools/.github/actions/report-coverage@v1:

  1. It merges the matrix artifacts server-side (actions/upload-artifact/merge) but never downloads them into the workspace, so every coverage command ran against an empty directory.
  2. python -Iim includes -i. Each command exits non-zero, Python then drops into an interactive interpreter, reads EOF from the empty stdin, and exits 0 — so the job went green and the --fail-under=80 gate never fired once.

This PR replaces that job with per-matrix-job uploads to Codecov, adds Codecov components, and fills in the library's largest coverage gaps.

CI wiring

  • .github/workflows/tests.yml — each matrix job uploads its own coverage.xml via codecov/codecov-action@v5, tagged flags: <os>-<python-version>. Codecov merges the uploads for the commit, so the artifact round-trip and the whole coverage job are gone.
  • codecov.yml — new. project: auto with a 0.5% threshold, patch: 80%, comment only when coverage changes.
  • pyproject.tomlbranch = true moved into [tool.coverage.run] (it was --cov-branch on the hatch script only, so any bare coverage invocation measured something different), and --cov-report xml added to cov:test.
  • README.md — Codecov badge. .gitignorecoverage.xml.

Coverage

before after
traitlets/ 80.26% 87.07%
config/application.py 76% 94%
config/manager.py 0% 100%
config/sphinxdoc.py 0% 100%
utils/descriptions.py 70% 100%
utils/getargspec.py 35% 100%
utils/sentinel.py 79% 100%

manager.py and sphinxdoc.py had no tests at all — 117 statements. The Sphinx extension imports no Sphinx, so a stub with an add_object_type method exercises setup().

application.py's ~113 uncovered lines were nearly all the help machinery, and most of it was not untested: test_help_output and friends go through check_help_output, which spawns a subprocess that coverage doesn't follow. The lines ran; nothing recorded it. The new tests call emit_alias_help, emit_flag_help, emit_options_help, emit_subcommands_help, emit_help, emit_description, emit_examples, the print_* wrappers, start_show_config, load_config_environ, boolean_flag, get_config and launch_instance in process — which measures them, and pins the generator API downstream applications build on. What still isn't covered there is error paths that log and re-raise.

93 new tests, 675 total, passing on both current pytest and the pinned minimum (7.0.1).

tests/test_typing.py is omitted from measurement: mypy type-checks it and pytest-mypy-testing never executes the bodies, so all 454 lines read as uncovered. It's a fixture for the type checker, not dead code.

Components

component_management defines traitlets (traitlets/**) and tests (tests/**), each with its own project status. A component can only report on files in the report, so coverage measures the test suite too — a test file with uncovered lines is a test that never runs. That blends the headline number upward, which is why the split matters: traitlets is the one to watch.

Coverage is measured from the repository root rather than source = ["traitlets", "tests"]. That's deliberate: coverage xml writes each path relative to its source root, so with two roots both config/__init__.py files land in the XML under the same name. They're empty today and nothing collides — the first line of code added to tests/config/__init__.py would silently merge the two on Codecov's side.

Verified, not assumed

  • Codecov's API confirms the per-job uploads merge: state: complete, 22 sessions, 40 files on an earlier revision. The 40 vs 43 files measured locally is exactly the three empty __init__.py files under tests/, which coverage xml omits as zero-statement.
  • coverage xml normalises the filename attribute to forward slashes on every platform (coverage/xmlreport.py:190,207), which is why the Windows uploads line up with the rest.
  • codecov.yml validates against codecov.io/validate, components included.
  • Rebased onto 0ef86a9.

Notes for review

  • Codecov reads a couple of points below the CI number — it counts partial branches as uncovered where coverage.py gives partial credit.
  • CODECOV_TOKEN is not set. Uploads take the tokenless path, which works but is rate-limited and less reliable on fork PRs. Worth adding, then flipping fail_ci_if_error: true.
  • after_n_builds is deliberately absent. With one upload per matrix job it would have to equal the matrix size, and would silently report partial coverage the first time someone adds a Python version without bumping it. wait_for_ci (default true) already holds the comment and statuses until CI finishes.
  • The --fail-under gate is gone with the combine job; Codecov's project status replaces it.
  • The codecov/project checks compare against a November 2022 baseline (1077cfb), the last commit on main carrying a report. That comparison is measuring two different things and resolves itself once this lands and main gets a fresh report. Details in the thread below.
  • If coverage was a required status check in branch protection, the check no longer exists and will need removing — I can't read branch protection settings from here.

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@Carreau Carreau changed the title Refactor coverage reporting to use Codecov integration Make coverage reporting actually run, and send it to Codecov Aug 3, 2026

Carreau commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Status on the red checks — one was a real bug, now fixed; the other two are an artifact of enabling Codecov.

Test Minimum Versions — fixed in 2c4d4b6.

tests/config/test_sphinxdoc.py imported the extension's setup under its own name. pytest before 8.0 treats a module-level setup as an xunit setup hook and calls it once per test with the test module as its argument:

app.add_object_type("configtrait", "configtrait", objname="Config option")
E  AttributeError: module 'tests.config.test_sphinxdoc' has no attribute 'add_object_type'
app = <module 'tests.config.test_sphinxdoc' from '.../tests/config/test_sphinxdoc.py'>

Every test in the file errored. Only the minimum-versions job caught it, since it pins pytest 7.0 and the nose-style hooks were removed in pytest 8 — the other jobs run a newer pytest and passed. It is now imported as setup_extension, verified against pytest 7.0.1 (15 passed, 0 errors) and against the full suite on both pytest 7.0.1 and current.

codecov/project and codecov/project/traitlets — expected, and self-correcting on merge.

Both compare against 1077cfb, which is a merge commit from November 2022 — the last commit on main carrying a Codecov report, from the last time this repository used the service. That report predates branch coverage, so the comparison is measuring two different things:

check this PR base (Nov 2022) delta
codecov/project 89.63% 93.44% −3.81%
codecov/project/traitlets 82.67% 93.44% −10.78%

Nothing regressed — coverage of traitlets/ goes up in this PR, 80.26% → 84.59% as coverage.py measures it. The gap between 84.59% and Codecov's 82.67% is partial branches, which Codecov counts as uncovered.

I have deliberately not changed the targets to make these go green. target: auto is the right steady state, and the moment this merges, main gets a fresh report that every later PR compares against correctly. Exactly one PR is affected by the stale base: this one. The alternative — switching to absolute targets now and back to auto later — is churn for a one-time artifact.


Generated by Claude Code

claude added 5 commits August 3, 2026 19:01
The `coverage` job has been a no-op. `report-coverage` merges the matrix
artifacts server-side but never downloads them into the workspace, so
every command in it ran against an empty directory:

    + python -Iim coverage combine
    No data to combine
    + python -Iim coverage report --fail-under=80
    No data to report.

The job still passed, because `-Iim` includes `-i`: after each command
exits non-zero, Python drops into an interactive interpreter, reads EOF
from the empty stdin, and exits 0. So the 80% gate never once fired.

Replace it with explicit steps that download the `coverage-*` artifacts,
combine them, and publish the result to Codecov. The checkout the job
already does turns out to be load-bearing: merging the Windows data with
the POSIX data relies on `relative_files`, and coverage only maps a
recorded path onto a canonical one when that file exists on disk.

The floor is set to 78%, just under the 80.26% a single run measures
today, so that it stays a backstop rather than a tripwire. Codecov's
`project` status, at `target: auto`, is what ratchets coverage up.

Also move `branch` into `[tool.coverage.run]`. It was passed as
`--cov-branch` from the hatch script, which left the `coverage` CLI
invocations in CI measuring something subtly different from the pytest
run that produced the data.

Codecov needs `CODECOV_TOKEN` in the repository secrets; without it the
upload falls back to tokenless, which is rate-limited. `fail_ci_if_error`
is off until that secret exists.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qMpoL5cL6W4qCeCdGm26w
Codecov already merges every upload it receives for a commit, so the
artifact round-trip and the combine job were doing work the service
does anyway. Each matrix job now uploads its own coverage.xml, tagged
with a flag, which also gets the per-OS and per-version breakdown that
a single combined upload cannot show.

`coverage xml` writes the `filename` attributes with forward slashes
regardless of platform, so the Windows uploads line up with the rest
without the path mapping the combined data file needed.

Drop `after_n_builds`. It was correct at 1 for a single upload; with one
upload per matrix job it would have to track the matrix size, and would
silently report partial coverage the first time someone adds a Python
version without bumping it. `wait_for_ci` (true by default) already
holds the comment and the statuses until CI has finished.

This gives up the `--fail-under` gate that ran in the combine job.
Codecov's `project` status covers it, and is the stricter of the two.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qMpoL5cL6W4qCeCdGm26w
…nent

`manager.py` and `sphinxdoc.py` were both at 0% -- 117 statements that no
test ever touched. Neither needs much of a harness: the config manager is
JSON on disk, and the Sphinx extension turns out to import no Sphinx at
all, so a stub with an `add_object_type` method is enough to exercise
`setup()`. Both are now at 100%, branches included, along with the gaps in
`descriptions.py`, `getargspec.py`, `sentinel.py` and `bunch.py`.

The library goes from 80.26% to 84.59%.

For the components, coverage has to measure the test suite as well -- a
component can only report on files that are in the report. Codecov then
splits the two apart, since blending them gives a number (89.25%) that
mostly tracks how much of the test code runs rather than how much of the
library is covered.

Measuring from the repository root, rather than `source = ["traitlets",
"tests"]`, is deliberate. `coverage xml` writes each path relative to its
source root, so with two roots both `config/__init__.py` files land in the
XML under one name. Today they are empty and nothing collides; the first
line of code added to `tests/config/__init__.py` would silently merge the
two on Codecov's side. One root keeps the paths distinct.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qMpoL5cL6W4qCeCdGm26w
pytest before 8.0 treats a module-level `setup` as an xunit setup hook and
calls it once per test with the test module as its argument, so importing
the extension's `setup` into the test module's namespace turned every test
in the file into an error:

    AttributeError: module 'tests.config.test_sphinxdoc' has no attribute
    'add_object_type'

Only the minimum-versions job caught it, since it pins pytest 7.0 and the
nose-style hooks were removed in pytest 8. Verified against pytest 7.0.1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qMpoL5cL6W4qCeCdGm26w
`application.py` sat at 76% with ~113 uncovered lines, nearly all of them
the help machinery: emit_alias_help, emit_flag_help, emit_options_help,
emit_subcommands_help, emit_help, emit_description, emit_examples and the
print_* wrappers around them, plus start_show_config, load_config_environ,
boolean_flag, get_config and launch_instance.

Most of that is not actually untested. test_help_output and friends drive
the application through `check_help_output`, which spawns a subprocess, and
coverage does not follow the child. So the lines run, but nothing records
it. These tests call the emitters in process instead, one method at a time,
which measures them and pins the generator API that downstream applications
use. 40 tests, taking application.py from 76% to 94%.

The remaining misses there are error paths that re-raise after logging, and
would need a deliberately malformed alias or flag to reach.

tests/test_typing.py is now omitted. mypy type-checks it and
pytest-mypy-testing never executes the bodies, so all 454 lines read as
uncovered -- it is a fixture for the type checker, not dead code, and
counting it only depressed the number.

The library goes from 84.59% to 87.07%.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qMpoL5cL6W4qCeCdGm26w
@Carreau
Carreau force-pushed the claude/coverage-codecov-setup-892zlt branch from 2c4d4b6 to f1eed9f Compare August 3, 2026 19:04
os.environ upper-cases every key on Windows, so HELPAPP__Foo__name arrives
as HELPAPP__FOO__NAME. load_config_environ splits the trait name off the
end and assigns it unchanged, and Config rejects a key beginning with an
uppercase letter unless the value is another Config:

    ValueError: values whose keys begin with an uppercase char must be
    Config instances: 'NAME', DeferredConfigString('from-the-environment')

This is not specific to the names in the test. Every trait name is
upper-cased the same way, so no environment variable can set any trait on
Windows -- the "Warning, case sensitive!" note in the method is
understating it. That behaviour predates this branch; the tests merely
reached code that nothing had reached before.

Skipping keeps the tests honest about the limitation without asserting
that the broken behaviour is correct, and Linux and macOS still cover the
method. Fixing load_config_environ is a behaviour change, and belongs in
its own pull request.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019qMpoL5cL6W4qCeCdGm26w

Carreau commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

The Windows jobs went red on f1eed9f, and the cause is worth surfacing on its own: Application.load_config_environ cannot set any trait on Windows.

The three new tests for it failed on Windows 3.10, 3.13 and 3.14 with:

ValueError: values whose keys begin with an uppercase char must be Config instances:
  'NAME', DeferredConfigString('from-the-environment')

os.environ upper-cases every key on Windows, so HELPAPP__Foo__name arrives as HELPAPP__FOO__NAME. The method splits the trait name off the end:

_, *path, key = k.split("__")
section = new_config
for p in path:
    section = section[p]
setattr(section, key, DeferredConfigString(v))

key is therefore always upper-cased, and Config.__setattr__ rejects an uppercase key whose value isn't a Config. This isn't specific to the names in my test — every trait name is upper-cased identically, so on Windows any environment variable matching the app's prefix raises instead of configuring anything. The # Warning, case sensitive ! comment in the method is understating the consequence.

This predates the branch. Nothing in the suite reached load_config_environ before, which is exactly why it sat in the uncovered block — 68139b7 skips the three tests on Windows rather than asserting that the current behaviour is correct, and Linux and macOS still cover the method. A fix would be a behaviour change (case-insensitive lookup, or lower-casing the trait component on Windows) and belongs in its own pull request — happy to open one if you'd like.


Generated by Claude Code

@Carreau
Carreau merged commit 1a0ca47 into main Aug 3, 2026
30 of 32 checks passed
@Carreau
Carreau deleted the claude/coverage-codecov-setup-892zlt branch August 3, 2026 19:12
@Carreau Carreau added this to the 5.17 milestone Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants